-
Notifications
You must be signed in to change notification settings - Fork 17
WIP Proposal: Unified authoring app
#454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
authoring app
|
After this transition, I think I'd want to eventually refactor the |
|
I'm killing this auto-discovery code from the PR because I don't think the convenience is worth the loss of code introspection in editors and CI, but I want to preserve it here in case we need to write some applet-traversing code again: """
This was an attempt to make cute and clever code to dynamically discover and
import all applet modules of a certain type for aggregation purposes (e.g. the
authoring/models.py file importing from all applet models.py files). This code
actually does work, but I only realized after buildling it that it breaks
editor introspection/autocomplete, which makes the cost far too high for this
convenience.
"""
import functools
import importlib
import inspect
import pathlib
def auto_import(module_name):
"""Auto-import all modules with a given name in subdirs of applets."""
caller_frame_info = inspect.stack()[1]
caller_module = inspect.getmodule(caller_frame_info[0])
caller_module_name = caller_module.__name__
# converts openedx_learning.authoring.models -> openedx_learning.authoring
import_base = ".".join(caller_module_name.split(".")[:-1])
caller_filepath = caller_frame_info.filename
caller_dir = pathlib.Path(caller_filepath).resolve().parent
applets_dir = caller_dir / "applets"
module_paths = applets_dir.rglob(f"{module_name}.py")
relative_paths = [
module_path.relative_to(caller_dir) for module_path in module_paths
]
all_modules_dict = {}
for relative_path in sorted(relative_paths):
module_import_name = f"{import_base}." + ".".join(relative_path.parts)[:-3]
module = importlib.import_module(module_import_name)
module_dict = vars(module)
if '__all__' in module_dict:
module_dict_to_add = {
key: module_dict[key]
for key in module_dict['__all__']
}
else:
module_dict_to_add = {
key: val
for (key, val) in module_dict.items()
if not key.startswith('_')
}
all_modules_dict.update(module_dict_to_add)
return all_modules_dict
auto_import_models = functools.partial(auto_import, "models")
auto_import_api = functools.partial(auto_import, "api")
auto_import_admin = functools.partial(auto_import, "admin") |
Could this be done using Django's "squashed migration" feature instead of custom logic?
I'm not a fan of "applet", so I'd suggest "[sub]modules". Or perhaps better, continue to call them "apps" and just rename the larger thing - "umbrella app", "super-app", "domain", etc. |
This is a wacky proposal that I've been kicking around in my head since I started working seriously on #452, and ran into issues with renaming the app and/or moving models around.
What is this?
This PR refactors the repo to combine all authoring apps (publishing, components, content, collections, etc.) into a single
authoringapp from Django's point of view. But the boundaries previously set up by the apps still exist inopenedx_learning.apps.authoring.applets.*which has different packages forcomponents,collections, and all the rest.Each of these sub-apps still have their own
models.pyandadmin.pyfiles, though these are all stitched together by the higher levelauthoringfiles. So for instance,openedx.apps.authoring.modelsimports everything from the sub-apps.We could make utility wrappers to make this more convenient later.(Edit: Introspection magic breaks code autocomplete.)It would look like:
Why do this?
I still believe that having small, discrete app-like things is useful for controlling complexity, and I don't want to give that up. That being said, refactoring is made much harder when we want to try to either change the names of real Django apps (e.g.
contentstomedia) or if we want to move models around (e.g.Container/ContainerVersionleaving thepublishingapp to go to a newcontainersapp). Having all the models be in one namespace will make shifting the boundaries between them much easier.This will have a few other minor benefits. We can do a top level
api.pyfile forauthoringin a way that's consistent with other apps in the system. It sort of sets up the umbrellaauthoringapp as the holder of the public interface. It also makes it less cumbersome to enter in the list of apps.On the downside, there's less consistency in terms of what goes where. Management commands, migrations, and app initialization code has to go in the root
authoringapp.How is it working?
The other apps disappear entirely, and are replaced by one
authoringapp. Theauthoringapp's initial migration tries to be smart–it will create a whole new set of tables if it's a new database, but if it detects an up-to-date Ulmo install of app migrations, it will take over the model state for all those models without running any database operations. (The second migration then changes all the table names to start withoel_authoring.) Being just post-release is a perfect time to do this pretty radical realignment.What to call it?
I'm not sure what to call this practice. I'm currently going with
authoringbeing an "umbrella app", and the small things being "applets"... but I'm open to suggestions. In DDD terms, theauthoringapp would be a subdomain, and the individual applets would be bounded contexts—but "Context" already means something in Django, so I thought it would be too confusing to borrow that terminology.If we're just moving things around, why are there so many more lines removed than added?
It's because we're deleting
app.pyand migration files for the individual apps.